Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

projector: always check root logdir for embeddings #3694

Merged
merged 1 commit into from
Jun 10, 2020

Conversation

wchargin
Copy link
Contributor

@wchargin wchargin commented Jun 2, 2020

Summary:
Previously, the projector plugin would look for checkpoints in the root
logdir only if there were no other logs. This led to a confusing
discontinuity: if you add logs to a subdirectory of a logdir with only
projector data, suddenly your projector data would disappear in the UI.
This patch changes the projector plugin to always look in the root log
directory for checkpoints. Fixes #3686.

Test Plan:
Three cases to test:

  • checkpoints in root logdir, no other data
  • checkpoints and summary data in root logdir
  • checkpoints in root logdir, summary data in non-root run

Verify that each of these cases lets the projector plugin render and
that the data shows up only once (i.e., it’s not duplicated).

wchargin-branch: projector-always-check-root

Summary:
Previously, the projector plugin would look for checkpoints in the root
logdir only if there were no other logs. This led to a confusing
discontinuity: if you add logs to a subdirectory of a logdir with only
projector data, suddenly your projector data would disappear in the UI.
This patch changes the projector plugin to _always_ look in the root log
directory for checkpoints. Fixes #3686.

Test Plan:
Three cases to test:

  - checkpoints in root logdir, no other data
  - checkpoints and summary data in root logdir
  - checkpoints in root logdir, summary data in non-root run

Verify that each of these cases lets the projector plugin render and
that the data shows up only once (i.e., it’s not duplicated).

wchargin-branch: projector-always-check-root
wchargin-source: 7fa825034ad74f83d4aab2bea10d2d6cd6485663
@wchargin wchargin merged commit b5e8e64 into master Jun 10, 2020
@wchargin wchargin deleted the wchargin-projector-always-check-root branch June 10, 2020 07:00
wchargin added a commit that referenced this pull request Dec 23, 2020
Summary:
This rolls forward #4479 (see that commit for context) with changes due
to Google-internal, non-TensorBoard code that monkey patches some of the
projector internals:

  - The `_run_paths` field is now updated only during `_update_configs`,
    to ensure that each change to `_run_paths` gives `_configs` the
    opportunity to update.
  - Configs/run paths are no longer updated in `__init__`, since that
    may be expensive and involve file access (even with a multiplexer).
  - The `_run_paths` field is now always a `dict` rather than `None`,
    fixing an existing bug wherein accessing `configs` on a plugin with
    no multiplexer would raise an `AttributeError`.

This reverts commit 4c9a699, and
therefore reinstates commit 638014e.

Test Plan:
Test plan from #4479 still passes, and a test sync now passes as well.
Also tested pointing the projector at a logdir with no runs but with
checkpoint data at root, as fixed in #3694; this worked in the original
version of this change (#4479), but it works now, too.

wchargin-branch: projector-fix-aliasing
wchargin-source: 0cb7e9c3917d5dc9f00616e35aa5bf756a51dea0
wchargin added a commit that referenced this pull request Jan 5, 2021
Summary:
This rolls forward #4479 (see that commit for context) with changes due
to Google-internal, non-TensorBoard code that monkey patches some of the
projector internals:

  - The `_run_paths` field is now updated only during `_update_configs`,
    to ensure that each change to `_run_paths` gives `_configs` the
    opportunity to update.
  - Configs/run paths are no longer updated in `__init__`, since that
    may be expensive and involve file access (even with a multiplexer).
  - The `_run_paths` field is now always a `dict` rather than `None`,
    fixing an existing bug wherein accessing `configs` on a plugin with
    no multiplexer would raise an `AttributeError`.

This reverts commit 4c9a699, and
therefore reinstates commit 638014e.

Test Plan:
Test plan from #4479 still passes, and a test sync now passes as well.
Also tested pointing the projector at a logdir with no runs but with
checkpoint data at root, as fixed in #3694; this worked in the original
version of this change (#4479), but it works now, too.

Test sync: <http://cl/350223680>.

wchargin-branch: projector-fix-aliasing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Projector doesn't work if I'm storing scalars too
3 participants